Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error messages for ERC721 and 1155 #3254

Merged
merged 5 commits into from
May 27, 2022
Merged

Improve error messages for ERC721 and 1155 #3254

merged 5 commits into from
May 27, 2022

Conversation

kanedaaaa
Copy link
Contributor

Fixes #3120

As discussed in the issue, the ERC721 approve error message was misleading.

All tho, we can say the same thing on a very similar error message:
"ERC1155: the caller is not owner nor approved" or "ERC721: approve caller is not owner nor approved for all"

I didn't modify this one since the first error message fix was accepted because it's more meaningful and shorter than the previous one, which won't apply in this. And also, there is an open PR that fixes the same issue. But still, would be nice if I could add token in mentioned messages.

P.S The PR mentioned above also goes against my logic (removing the "transfer" keyword in my version) so definitely check it.

  • Tests
  • Documentation
  • Changelog entry

Amxx
Amxx previously approved these changes Mar 9, 2022
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Amxx Amxx requested a review from frangio March 9, 2022 16:42
@Amxx
Copy link
Collaborator

Amxx commented Mar 9, 2022

Wait, you are missing ERC1155Burnable

@kanedaaaa
Copy link
Contributor Author

kanedaaaa commented Mar 9, 2022

@Amxx Refer to what i said above. These messages aren't consistent. Thus, I had to modify only ones that contained "transfer" before "caller". I pointed out that there is another PR that completely modified every single "token owner" error message.

If it's on me, I would modify the remaining unmodified error messages with the token keyword.

I can add edits to make everything more consistent if you wish so.

@Amxx
Copy link
Collaborator

Amxx commented Mar 9, 2022

In ERC1155 there is ERC1155: caller is not owner nor approved. IMO the confusion with a possible contract owner also exists

@kanedaaaa
Copy link
Contributor Author

Yes, I will definitely make the messages more consistent so wait before reviewing this PR, will add changes soon.

@kanedaaaa kanedaaaa requested a review from Amxx March 9, 2022 17:21
@frangio frangio added this to the 4.7 milestone Mar 21, 2022
@frangio frangio changed the title fix: improve error messages for ERC721 and 1155 Improve error messages for ERC721 and 1155 Mar 30, 2022
@frangio
Copy link
Contributor

frangio commented Mar 30, 2022

This PR is now increasing the length of revert reasons, which we initially thought would just be shortened in #3120 (comment), so it now needs some more analysis to see how this is affecting bytecode size before we agree to merge.

@kanedaaaa
Copy link
Contributor Author

kanedaaaa commented Apr 12, 2022

Yes, i think there was bit of misunderstanding here regarding to that. My first commit shortened the error messages, so we can just go back there since other changes that made messages longer is in second commit.

BTW, i was just thinking and since we are discussing same thing: what if there were error codes, rather than full revert message, such as:

ERC1155: ERR_001 - Then, we can write down in docs what this ERR_001 means, with more explanation and examples.

Good example from PancakeSwap: https://docs.pancakeswap.finance/help/troubleshooting

Shortens the bytecode, and it's more organised in general. But anyways, custom error messages are on the way and we might as well wait for it to be fully implementable. As i know there is still no way to trigger them with require()

@frangio
Copy link
Contributor

frangio commented May 27, 2022

We're catching up on pending PRs. Sorry for the delay @kanedaaaa.

Following up on my last point, I realized that the change is still net positive because in this PR you are unifying what were different revert reasons into the same revert reason and this gets optimized by the compiler so the constant is present only once in the code.

We used to have the goal to have different revert reasons for every different error condition so that the source of an error would be easy to identify, but I don't think this is a strong argument anymore because tooling has evolved and e.g. Hardhat can show the real source of an error.


Regarding your point about error codes: yes, it's a good pattern, but we are also now seeing projects start adopting custom errors which are like "real" error codes plus with superpowers (parameters) and this is something we plan to adopt soon as well. (#2839)

frangio
frangio previously approved these changes May 27, 2022
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@frangio frangio enabled auto-merge (squash) May 27, 2022 00:35
@frangio frangio disabled auto-merge May 27, 2022 00:41
@frangio frangio enabled auto-merge (squash) May 27, 2022 00:45
@frangio frangio merged commit 488dd56 into OpenZeppelin:master May 27, 2022
@kanedaaaa kanedaaaa deleted the fix/improve-error-messages-#3120 branch October 18, 2022 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve clarity of error messages in functions checking token ownership
3 participants